lib/interfaces/index.ts (2)

149-156: Tighten TransactionBuilder.chainId to non-optional for internal consistency.

Since transaction(props: TransactionParams) now requires chainId, any resulting TransactionBuilder created from it should always have chainId. Keeping it optional increases nullish checks downstream.

Apply:

 export interface TransactionBuilder {
-  chainId?: number;
+  chainId: number;
   to?: string;
   value?: bigint | string;
   data?: string;
   transactionName?: string;
   batchName?: string;
 }

If there are legitimate flows that produce “working” builders before validation, consider using a separate “DraftTransactionBuilder” for pre-validation and “TransactionBuilder” (with required chainId) post-validation.

---

177-201: Model success/error results as discriminated unions to improve DX and safety.

Today TransactionEstimateResult/TransactionSendResult use booleans with many optionals. A discriminated union avoids accessing undefined fields on failures and lets you require chainId on success while keeping it optional on failure.

Example (outside this diff):

type EstimationSuccess = {
  isEstimatedSuccessfully: true;
  to?: string;
  value?: string;
  data?: string;
  chainId: number;
  cost?: bigint;
  userOp?: UserOp;
};

type EstimationFailure = {
  isEstimatedSuccessfully: false;
  errorType: 'ESTIMATION_ERROR' | 'VALIDATION_ERROR';
  errorMessage: string;
};

export type TransactionEstimateResult = EstimationSuccess | EstimationFailure;

A similar union can be defined for send results using isSentSuccessfully.

example/src/App.tsx (1)

42-45: Avoid repeating the magic number 137 for chainId.

The example hardcodes chainId: 137 in many places. Define a const CHAIN_ID = 137 once and reuse it. This reduces churn if you switch networks and keeps example code tight.

Apply pattern:

- kit.transaction({ chainId: 137, to: '0x...', value: '...' });
+ const CHAIN_ID = 137; // near the top, once
+ kit.transaction({ chainId: CHAIN_ID, to: '0x...', value: '...' });

Also applies to: 61-64, 91-94, 110-113, 259-262, 280-283, 304-307, 314-317, 338-341, 348-351, 366-369, 403-406, 438-438, 451-454, 491-494, 529-532, 551-554, 567-570, 576-579

__tests__/EtherspotTransactionKit.test.ts (2)

223-229: Covers non-integer chainId; add negative/zero case for completeness.

You already validate non-integer (1.5). Consider a quick additional test for chainId <= 0 to catch invalid ranges.

You can append:

+    it('should throw error for non-positive chainId', () => {
+      expect(() => {
+        transactionKit.transaction({
+          to: '0x1234567890123456789012345678901234567890',
+          chainId: 0,
+        });
+      }).toThrow('transaction(): chainId must be a valid number.');
+      expect(() => {
+        transactionKit.transaction({
+          to: '0x1234567890123456789012345678901234567890',
+          chainId: -1,
+        });
+      }).toThrow('transaction(): chainId must be a valid number.');
+    });

---

4-6: Typo in comment: “EtherspotPovider”.

Nit, but easy to clean up.

-// EtherspotPovider
+// EtherspotProvider

lib/TransactionKit.ts (7)

114-115: Use nullish coalescing for chainId fallback in getWalletAddress()

Prefer ?? over || for consistency and to avoid falsey pitfalls (even though 0 isn’t a valid chainId, this keeps style consistent across the file).

-    const walletAddressChainId = chainId || this.etherspotProvider.getChainId();
+    const walletAddressChainId = chainId ?? this.etherspotProvider.getChainId();

---

98-111: Docs/code mismatch: method never throws but JSDoc says it does

getWalletAddress() catches errors and returns undefined, yet the JSDoc advertises a throw. Align the comment with behavior.

-   * @throws {Error} If the SDK fails to initialize or the address cannot be fetched due to a critical error.
+   * @throws Never. Errors from SDK init/address retrieval are caught; the method logs and returns undefined.

---

176-190: Good: chainId made mandatory with runtime validation; add positive-integer guard

Solid move to require chainId and validate type/integery. Consider rejecting non-positive values to catch accidental zeros/negatives early.

     if (typeof chainId !== 'number' || !Number.isInteger(chainId)) {
       this.throwError('transaction(): chainId must be a valid number.');
     }
+    if (chainId <= 0) {
+      this.throwError('transaction(): chainId must be a positive integer.');
+    }

---

662-707: Reduce duplication in SDK prepare/clear/add/estimate/send flows

The same sequence is repeated across single and batch paths: get SDK for a chain -> clear batch -> add ops -> estimate -> totalGas -> send. Extract helpers to DRY the logic and centralize error handling.

Example additions inside the class to reuse:

private async prepareSdk(chainId: number, forceNew = true): Promise<ModularSdk> {
  return this.etherspotProvider.getSdk(chainId, forceNew);
}

private async addTxToBatch(
  sdk: ModularSdk,
  tx: { to?: string; value?: bigint | string; data?: string }
): Promise<void> {
  await sdk.clearUserOpsFromBatch();
  await sdk.addUserOpsToBatch({
    to: tx.to || '',
    value: tx.value?.toString(),
    data: tx.data ?? '0x',
  });
}

private async estimateCost(
  sdk: ModularSdk,
  estimateArgs: { paymasterDetails?: unknown; gasDetails?: unknown; callGasLimit?: unknown },
  userOpOverrides?: Record<string, unknown>
): Promise<{ userOp: any; totalGas: bigint; cost: bigint }> {
  const base = await sdk.estimate(estimateArgs);
  const userOp = { ...base, ...(userOpOverrides || {}) };
  const totalGas = BigInt((await sdk.totalGasEstimated(userOp)).toString());
  const maxFeePerGas = BigInt(userOp.maxFeePerGas.toString());
  return { userOp, totalGas, cost: totalGas * maxFeePerGas };
}

Then each call site collapses to a small sequence, improving readability and testability.

Also applies to: 869-938, 1140-1215, 1412-1577

---

399-405: Typo in docstring: “trakit” → “kit”

Minor doc polish.

-   * Removes the currently selected transaction or batch from the trakit.
+   * Removes the currently selected transaction or batch from the kit.

---

128-129: Optional: cache/reuse SDK instances per chain during batch processing

You’re forcing new instances to avoid state pollution, which is safe but may be heavy if many batches share the same chain. Consider a short-lived per-call cache keyed by chainId to reuse within a single estimateBatches()/sendBatches() invocation.

I can sketch a scoped cache map (cleared at method exit) if you want to explore this optimization.

Also applies to: 663-667, 874-879, 1140-1146, 1429-1433

---

1816-1847: Align logging and confirm return type in getTransactionHash

The SDK’s getUserOpReceipt already returns Promise<string | null>, so no additional extraction logic is required. However, we should replace console.error/console.warn with our log() helper to respect debugMode and keep output consistent.

– File: lib/TransactionKit.ts
– Method: getTransactionHash (lines 1816–1847)

Suggested diff:

   public async getTransactionHash(
     userOpHash: string,
     txChainId: number,
     timeout: number = 60 * 1000,
     retryInterval: number = 2000
   ): Promise<string | null> {
     const etherspotModularSdk = await this.getSdk(txChainId);

     let transactionHash: string | null = null;
     const timeoutTotal = Date.now() + timeout;

     while (!transactionHash && Date.now() < timeoutTotal) {
       await new Promise<void>((resolve) => setTimeout(resolve, retryInterval));
       try {
-        transactionHash =
-          await etherspotModularSdk.getUserOpReceipt(userOpHash);
+        transactionHash = await etherspotModularSdk.getUserOpReceipt(userOpHash);
       } catch (error) {
-        console.error(
-          'Error fetching transaction hash. Please check if the transaction has gone through, or try to send the transaction again:',
-          error
-        );
+        log(
+          'getTransactionHash(): error fetching tx hash',
+          error,
+          this.debugMode
+        );
       }
     }

     if (!transactionHash) {
-      console.warn(
-        'Failed to get the transaction hash within time limit. Please try again'
-      );
+      log(
+        'getTransactionHash(): failed to get tx hash within time limit',
+        { userOpHash, timeout, retryInterval },
+        this.debugMode
+      );
     }

     return transactionHash;
   }

Note: If you ever need the full transaction receipt, simply pass the hash into your provider, e.g.:

const txHash = await modularSdk.getUserOpReceipt(uoHash);
if (txHash) {
  const receipt = await provider.getTransactionReceipt(txHash);
  // …use full receipt here
}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

package.json (1)

4-4: Release Version 2.0.3 Verified

• Confirmed package.json version is set to 2.0.3.
• CHANGELOG.md contains the header ## [2.0.3] – 2025-08-22.

All checks pass—please proceed with tagging the v2.0.3 release.

lib/interfaces/index.ts (1)

239-243: Make chainId required in TransactionParams — aligned with PR objective.

Type-level enforcement here is good and will catch mistakes at compile time.

__tests__/EtherspotTransactionKit.test.ts (1)

196-206: Good: defaulting optional tx fields while requiring chainId.

Test now confirms value/data defaults with explicit chainId. This matches the new contract.

lib/TransactionKit.ts (6)

128-134: LGTM: Modular SDK variable naming is now consistent

The etherspotModularSdk naming is consistent here and matches usage elsewhere. Good cleanup.

---

627-629: LGTM: chainId resolution uses nullish coalescing

The error path now prefers the transaction’s chainId and only falls back to the provider when undefined/null. Matches the PR objective.

---

896-904: send(): no support for gasDetails/callGasLimit during inline estimation

estimate() accepts gasDetails/callGasLimit, but send()’s inline estimation forwards only paymasterDetails. If this is intentional, all good; otherwise consider adding the same knobs for parity.

---

972-980: LGTM: error-path chainId uses tx chainId first, falls back to provider

Consistent with the goal of honoring transaction-specified chain context.

---

996-1003: Nice: preserve tx details before clearing state

Capturing successResult avoids losing fields after clearWorkingState(). Clean and safe.

Also applies to: 1021-1027

---

1230-1239: Per-tx cost equals full batch total; confirm this UX

Each transaction in a batch receives cost: totalCost. This may be confusing to consumers expecting a per-tx allocation. If intended, consider documenting it explicitly or returning both totalCost (batch-level) and allocatedCost (per-tx split).

I can draft a small change to include allocatedCost = totalCost / N for display while keeping totalCost in the batch result—let me know if you want that.

Also applies to: 1626-1636

CHANGELOG.md (2)

3-8: Potential semver mismatch: breaking change in a patch release (2.0.3)

If the API now throws when chainId is omitted, that’s a backward-incompatible change and typically warrants a major version bump (e.g., 3.0.0), or at minimum a clearly labeled “Breaking Changes” section with migration notes. Please confirm intended semver strategy.

Run to validate impact and decide on versioning:

#!/bin/bash
set -euo pipefail

echo "=== Check if chainId is still defaulted anywhere ==="
rg -nP --type ts '(?s)\btransaction\s*\(\s*\{\s*[^}]*\bchainId\s*=\s*' -C2 || true

echo
echo "=== Check if TransactionParams still allows optional chainId ==="
rg -nP --type ts '\binterface\s+TransactionParams\b(?s).*?\bchainId\?' -C2 || true

echo
echo "=== Tests expecting default chainId behavior ==="
rg -nP --glob '*test*' 'expect\(.*chainId.*\)\.to(Be|Equal)\(\s*1\s*\)' -n -C2 || true

echo
echo "=== Residual typos of etherspotModulaSdk ==="
rg -n 'etherspotModulaSdk' -n -C2 || true

---

5-8: Reclassify 2.0.3 notes as Breaking Changes and expand scope to include send()/estimate() requirement

Given the API now requires chainId (per PR description), this is a breaking change and should not be listed under “Added Changes.” Also, the PR states chainId is mandatory for single-transaction send()/estimate() calls—please capture that explicitly.

Apply:

-### Added Changes
-
-- `chainId` is mandatory in the `transaction()` method.
-- For `send` and `estimate`, the `etherspotModularSdk` is initialized using the transaction's `chainId` rather than the provider's `chainId`.
+### Breaking Changes
+- `transaction()`: `chainId` is now required; calls without it will throw.
+- `send()`/`estimate()` (single-transaction mode): `chainId` must be provided on the transaction.
+
+### Added Changes
+- The SDK now initializes `etherspotModularSdk` using the transaction's `chainId` instead of the provider's `chainId`.

CHANGELOG.md (2)

7-8: Add a Fixed section for the etherspotModularSdk typo and tighten wording

The PR mentions a typo fix; document it. Minor copy edits improve clarity.

Apply:

 ## [2.0.3] - 2025-08-22
@@
-### Added Changes
+### Added Changes
@@
-- For `send` and `estimate`, the `etherspotModularSdk` is initialized using the transaction's `chainId` rather than the provider's `chainId`.
+- The SDK initializes `etherspotModularSdk` using the transaction's `chainId` rather than the provider's `chainId`.
+
+### Fixed
+- Corrected a typo to consistently use `etherspotModularSdk`.

---

3-8: Add a short Migration note to reduce consumer friction

A brief snippet helps users adopt 2.0.3 safely.

Apply:

 ## [2.0.3] - 2025-08-22
@@
+### Migration
+- Add `chainId` to all `.transaction({...})` calls.
+- For single-transaction `send()` and `estimate()`, ensure the transaction object includes `chainId`.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.